Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Build: Give linting sub-commands a directory to run against #17545

Merged
merged 1 commit into from
Mar 7, 2022

Conversation

IanVS
Copy link
Member

@IanVS IanVS commented Feb 22, 2022

Issue: #17542

What I did

I was very confused when running yarn lint:js did not result in any files being linted. I've now realized that the target (.) is specified in the parent command, yarn lint. It seems to me like commands should do the thing they say they're doing, without needing extra arguments in order to function. So, I've moved the . into the linting subcommands here. I'm happy to close this out if there are reasons to keep it apart, but it was definitely confusing to me.

How to test

  • Is this testable with Jest or Chromatic screenshots?
  • Does this need a new example in the kitchen sink apps?
  • Does this need an update to the documentation?

You can run yarn lint:js --debug to verify that it is indeed running against files now.

@IanVS IanVS added the maintenance User-facing maintenance tasks label Feb 22, 2022
@nx-cloud
Copy link

nx-cloud bot commented Feb 22, 2022

☁️ Nx Cloud Report

CI ran the following commands for commit 7b03831. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this branch


✅ Successfully ran 1 target

Sent with 💌 from NxCloud.

@IanVS
Copy link
Member Author

IanVS commented Feb 22, 2022

ohhhhhh, I see, the . is in yarn lint, not within yarn lint:js. That's confusing, since I was expecting to be able to run just the subcommand without needing to supply extra arguments.

@IanVS IanVS force-pushed the 17542-turn-on-eslint branch from 53349bc to 7b03831 Compare February 22, 2022 15:09
@IanVS IanVS changed the title Give ESLint a directory to run against Give linting sub-commands a directory to run against Feb 22, 2022
Copy link
Contributor

@MichaelArestad MichaelArestad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @IanVS!

@shilman shilman changed the title Give linting sub-commands a directory to run against Build: Give linting sub-commands a directory to run against Mar 7, 2022
@shilman shilman merged commit 7c87145 into next Mar 7, 2022
@shilman shilman deleted the 17542-turn-on-eslint branch March 7, 2022 15:31
@IanVS
Copy link
Member Author

IanVS commented Mar 7, 2022

Note, this does mean that it's a bit trickier to run linting on a specific file or folder. However, that's a bit of an anti-pattern anyway when caching is enabled, because it will remove your cache file if you do not lint the same thing each time. So, it's usually faster to just run the lint-everything command, than to change what you're linting and keep blowing away the cache. So, I stand behind this change, but wanted to explain the reasoning in case anyone is used to running yarn lint:js path/to/my/file.ts.

If anyone does want to run against a single file (maybe because there are too many warnings in other files?), it's still possible with yarn cross-env NODE_ENV=production eslint --cache --cache-location=.cache/eslint --ext .js,.jsx,.json,.html,.ts,.tsx,.mjs --report-unused-disable-directives path/to/my/file.ts. I'd be happy to discuss other alternatives as well, like adding a separate script, if this is a common workflow.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
maintenance User-facing maintenance tasks
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants